feat: extra checks on signatures/pubkeys + check the signature first in antehandle #18194

bizk · 2023-10-20T20:49:23Z

Description

This PR add checks on antehanler where pubkeys are being used. It check if secp keys are on curve, but not ed since these are not supported yet. It also add tests for cases where keys are or not in the respective curve.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

Summary by CodeRabbit

New Features
- Enhanced the Info struct to support additional information through the ExtraInfo field.
- Updated the parsing of the chain-id from the genesis file using a streaming JSON parser.
Bug Fixes
- Ensured that the buf export command exports all proto dependencies.
- Removed the hardcoded gRPC address to localhost in the server module.
- Consistently set the viper prefix in the client and server, defaulting to the binary name for both.
Security Improvements
- Added key checks on signatures antehandle in the crypto and x/auth modules.
Tests
- Modified and added new test cases to validate different key types and their behavior.

…KeysChecks

coderabbitai · 2023-10-23T16:36:40Z

Walkthrough

The changes in this update focus on improving the security and functionality of the Cosmos SDK. Key updates include the introduction of a function to verify if a public key is on the curve, the addition of new keypair generation functions, and modifications to test functions. Import statements have been updated and reordered, unused imports and comments have been removed, and function parameters have been modified in some files. The overall goal is to enhance the codebase's security, maintainability, and test coverage.

Changes

File(s)	Summary
`crypto/keys/secp256k1/secp256k1.go`	Import statement and function call updates related to the `secp256k1` package.
`testutil/testdata/tx.go`	Addition of new keypair generation functions and import statement updates.
`x/auth/ante/sigverify.go` and `x/auth/ante/sigverify_test.go`	Introduction of a function to verify if a public key is on the curve and updates to test functions.
`tests/integration/.../deterministic_test.go`	Modifications to test functions, including changes in the order of function call arguments.
`testutil/testdata/...`	Removal of unused imports and comments, reordering of import statements, and updates to function parameters.
`CHANGELOG.md`	Documentation of updates related to the `buf export` command, the Info struct, chain-id parsing, key checks on signatures antehandle, and setting the viper prefix.

🐇 "As the leaves fall and the winds blow, 🍂🍁
Code changes come, in a steady flow. 💻🔄
Security enhanced, tests refined, 🛡️✅
Leaving no room for bugs behind. 🐛❌
Celebrate the changes, for they bring delight, 🎉😊
As we code away into the night!" 🌙💫

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai help to get help.
@coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

… into feature/anteKeysChecks

coderabbitai

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 2f23e8a and f5fa7cd.

Files selected for processing (4)

crypto/keys/secp256k1/secp256k1.go (4 hunks)
testutil/testdata/tx.go (3 hunks)
x/auth/ante/sigverify.go (6 hunks)
x/auth/ante/sigverify_test.go (3 hunks)

Files skipped from review due to trivial changes (1)

testutil/testdata/tx.go

Additional comments: 13

crypto/keys/secp256k1/secp256k1.go (4)

12-12: The import statement github.com/decred/dcrd/dcrec/secp256k1/v4 is renamed to secp256k1dcrd. Ensure that all references to the old import name secp256k1 have been updated to the new name secp256k1dcrd throughout the codebase.

42-42: The function call secp256k1.PrivKeyFromBytes(privKey.Key).PubKey() is updated to secp256k1dcrd.PrivKeyFromBytes(privKey.Key).PubKey(). This change is consistent with the updated import statement.

104-104: The function call secp256k1.S256().N is updated to secp256k1dcrd.S256().N. This change is consistent with the updated import statement.

132-132: The function call secp256k1.S256().N is updated to secp256k1dcrd.S256().N. This change is consistent with the updated import statement.

x/auth/ante/sigverify_test.go (3)

7-7: The import statement github.com/decred/dcrd/dcrec/secp256k1/v4 is renamed to secp256k1dcrd. Ensure that this change does not affect the rest of the codebase and that the new import name is used consistently throughout the code.

345-347: The SetPubKeyDecorator and IncrementSequenceDecorator are now chained together in the antehandler. This is a change from the previous version where only the IncrementSequenceDecorator was used. Ensure that this change is intentional and that it does not introduce any unexpected behavior.

370-468: A new test function TestAnteHandlerChecks is added. This function tests various scenarios with different key types, including keys that are not on the curve. This is a good addition as it improves the test coverage and ensures that the code behaves as expected in different scenarios.

x/auth/ante/sigverify.go (6)

8-10: The import statement github.com/decred/dcrd/dcrec/secp256k1/v4 is renamed to secp256k1dcrd. Ensure that this change does not affect the rest of the codebase and that the new import name is used consistently throughout the code.

99-101: The function verifyIsOnCurve is called to check if the public key is on the curve. This is a new security measure added to the code.

195-200: The function verifyIsOnCurve is called to check if the public key is on the curve. This is a new security measure added to the code.

264-302: The function verifyIsOnCurve is defined. It checks if the public key is on the curve. It supports different types of keys: ed25519, secp256k1, secp256r1, and multisig. For ed25519 keys, it returns an error as they are not supported. For secp256k1 and secp256r1 keys, it checks if they are on the curve. For multisig keys, it checks if all the keys are on the curve. If the key type is not recognized, it returns an error.

339-341: The function verifyIsOnCurve is called to check if the public key is on the curve. This is a new security measure added to the code.

433-439: The function verifyIsOnCurve is called to check if the public key is on the curve. This is a new security measure added to the code.

x/auth/ante/sigverify_test.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f5fa7cd and 6393a94.

Files selected for processing (2)

testutil/testdata/animal.go (1 hunks)
testutil/testdata/grpc_query.go (1 hunks)

Files skipped from review due to trivial changes (2)

testutil/testdata/animal.go
testutil/testdata/grpc_query.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6393a94 and fe9f412.

Files selected for processing (4)

tests/integration/bank/keeper/deterministic_test.go (17 hunks)
tests/integration/staking/keeper/deterministic_test.go (26 hunks)
testutil/testdata/grpc_query.go (2 hunks)
x/auth/keeper/deterministic_test.go (13 hunks)

Files skipped from review due to trivial changes (3)

tests/integration/bank/keeper/deterministic_test.go
tests/integration/staking/keeper/deterministic_test.go
testutil/testdata/grpc_query.go

Additional comments: 14

x/auth/keeper/deterministic_test.go (14)

123-123: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

135-135: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

152-152: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

177-177: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

184-184: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

195-195: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

211-211: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

221-221: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

230-230: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

241-241: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

304-304: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

316-316: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

351-351: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

363-363: The order of arguments in the DeterministicIterations function call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e9f9f4f and d2642bd.

Files selected for processing (1)

x/auth/ante/sigverify_test.go (3 hunks)

Additional comments: 2

x/auth/ante/sigverify_test.go (2)

7-7: The import statement github.com/decred/dcrd/dcrec/secp256k1/v4 is renamed to secp256k1dcrd. Ensure that this change does not affect the rest of the codebase and that the new import path is correct.

345-347: The pubKeyDecorator and IncrementSequenceDecorator are now chained together in the antehandler. This is a change from the previous version where only the IncrementSequenceDecorator was used. Ensure that this change is intentional and that it does not introduce any unexpected behavior.

x/auth/ante/sigverify_test.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between d2642bd and 46dbafb.

Files selected for processing (2)

tests/integration/bank/keeper/deterministic_test.go (17 hunks)
tests/integration/staking/keeper/deterministic_test.go (26 hunks)

Files skipped from review due to trivial changes (2)

tests/integration/bank/keeper/deterministic_test.go
tests/integration/staking/keeper/deterministic_test.go

x/auth/ante/sigverify.go

bizk · 2023-11-02T19:46:55Z

Done with the comments. Thx for the review @alexanderbez

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 46dbafb and b79aa2f.

Files selected for processing (3)

CHANGELOG.md (1 hunks)
x/auth/ante/sigverify.go (6 hunks)
x/auth/ante/sigverify_test.go (3 hunks)

Additional comments: 10

x/auth/ante/sigverify_test.go (4)

4-10: The new import secp256k1dcrd is used in the test TestAnteHandlerChecks to generate a private key that is not on the curve. This is a good test case to ensure that the code correctly handles invalid keys.

342-348: The IncrementSequenceDecorator is added to the antehandler chain. This decorator is responsible for incrementing the sequence of the account after the transaction is processed. This is a necessary step to prevent replay attacks.

359-368: The test cases for TestIncrementSequenceDecorator are well defined. They test the decorator in different contexts (recheck, check, simulate) and ensure that the sequence is correctly incremented.

370-471: The TestAnteHandlerChecks function tests different key types and their behavior. It includes a test case for a secp256k1 key that is not on the curve, which is a good test case to ensure that the code correctly handles invalid keys.

x/auth/ante/sigverify.go (6)

4-11: The new imports for secp256k1dcrd and errors are added correctly.

95-100: The verifyIsOnCurve function is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

191-199: The verifyIsOnCurve function is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

263-300: The verifyIsOnCurve function is implemented correctly. It checks if the public key is on the curve for supported key types. This is a good security practice to prevent invalid keys from being used.

335-340: The verifyIsOnCurve function is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

429-438: The verifyIsOnCurve function is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between b79aa2f and 69f65e0.

Files selected for processing (1)

x/auth/ante/sigverify.go (6 hunks)

Additional comments: 6

x/auth/ante/sigverify.go (6)

4-13: New import added for secp256k1dcrd package. Ensure that the package is compatible with the current project and its dependencies.

96-101: The verifyIsOnCurve function is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

192-200: The verifyIsOnCurve function is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

261-302: The verifyIsOnCurve function is added to verify if a public key is on the curve. It supports secp256k1, secp256r1, and multisig public keys. This is a good security practice to prevent invalid keys from being used.

336-345: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [336-439]

The verifyIsOnCurve function is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

430-439: The verifyIsOnCurve function is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 69f65e0 and 6740dea.

Files selected for processing (1)

CHANGELOG.md (1 hunks)

… into feature/anteKeysChecks

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6740dea and 7729e00.

Files selected for processing (1)

x/auth/ante/sigverify_test.go (3 hunks)

Additional comments: 3

x/auth/ante/sigverify_test.go (3)

359-367: The test cases are well structured and cover a variety of scenarios. However, it would be beneficial to add comments explaining what each test case is testing for better readability and maintainability.

396-400: The creation of an invalid secp256k1 key is done correctly. This is a good test case to ensure that the system correctly handles invalid keys.

434-470: The test cases are well structured and cover a variety of scenarios. However, it would be beneficial to add comments explaining what each test case is testing for better readability and maintainability.

x/auth/ante/sigverify_test.go

tac0turtle · 2023-11-07T13:53:32Z

CHANGELOG.md

@@ -69,7 +69,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#17733](https://github.com/cosmos/cosmos-sdk/pull/17733) Ensure `buf export` exports all proto dependencies
 * (version) [#18063](https://github.com/cosmos/cosmos-sdk/pull/18063) Include additional information in the Info struct. This change enhances the Info struct by adding support for additional information through the ExtraInfo field
 * [#18204](https://github.com/cosmos/cosmos-sdk/pull/18204) Use streaming json parser to parse chain-id from genesis file.
-
+* (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle


could we get more information on this, this is vague

Do you want me to change the changelog or just leave a comment in this PR?

tac0turtle · 2023-11-07T13:55:05Z

x/auth/ante/sigverify.go

@@ -250,6 +261,46 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool {
 	}
 }

+func verifyIsOnCurve(pubKey cryptotypes.PubKey) (err error) {


is there a performance overhead observed here?

Didn't observe any noticable changes, do you want me to do some benchmark testing?

tac0turtle

left two questions, please wait for two approvals before merging

bizk · 2023-11-08T21:47:40Z

Answered them @tac0turtle how ∂o you want to proceed?

bizk added 4 commits October 17, 2023 18:54

Renamed missleading variable name

9069bb6

added onCurve verification

fd1600f

added oncurve verification and tests

f6150a8

removed multisig verification

f74ef04

github-actions bot added the C:x/auth label Oct 20, 2023

bizk and others added 4 commits October 20, 2023 19:10

lint fix

767e1d1

Merge branch 'main' of github.com:Zondax/cosmos-sdk into feature/ante…

fd5a041

…KeysChecks

added back sequence check

68ef7eb

Merge branch 'main' into feature/anteKeysChecks

eda82b0

bizk and others added 8 commits October 24, 2023 13:07

fixed some tests and improved error handling

4715f21

Merge branch 'feature/anteKeysChecks' of github.com:Zondax/cosmos-sdk…

78d083a

… into feature/anteKeysChecks

lint-fix

b4edd4a

added multisig handling

f9ba818

lint fix

e4af4f0

Merge branch 'main' into feature/anteKeysChecks

f5fa7cd

lint fix

dcf7704

lint fix

b72057b

bizk marked this pull request as ready for review October 24, 2023 22:30

bizk requested a review from a team as a code owner October 24, 2023 22:30

Merge branch 'main' into feature/anteKeysChecks

078b642

github-prbot requested review from a team, facundomedica and julienrbrt and removed request for a team October 24, 2023 22:30

Merge branch 'feature/anteKeysChecks' of github.com:Zondax/cosmos-sdk…

6393a94

… into feature/anteKeysChecks

coderabbitai bot reviewed Oct 24, 2023

View reviewed changes

x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved

x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved

coderabbitai bot reviewed Oct 24, 2023

View reviewed changes

linter fixes

fe9f412

coderabbitai bot reviewed Oct 24, 2023

View reviewed changes

coderabbitai bot reviewed Oct 25, 2023

View reviewed changes

x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved

Merge branch 'main' into feature/anteKeysChecks

46dbafb

coderabbitai bot reviewed Oct 30, 2023

View reviewed changes

tac0turtle assigned alexanderbez, facundomedica and testinginprod Oct 30, 2023

alexanderbez reviewed Oct 30, 2023

View reviewed changes

x/auth/ante/sigverify.go Outdated Show resolved Hide resolved

x/auth/ante/sigverify.go Outdated Show resolved Hide resolved

x/auth/ante/sigverify.go Outdated Show resolved Hide resolved

bizk and others added 3 commits November 2, 2023 15:20

Merge branch 'main' into feature/anteKeysChecks

4761682

Fixed comments

5dcd350

added changelog.md

b79aa2f

bizk requested a review from alexanderbez November 2, 2023 19:46

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

lint fix

69f65e0

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

bizk and others added 2 commits November 3, 2023 10:43

small fix on test

12a79af

Merge branch 'main' into feature/anteKeysChecks

6740dea

coderabbitai bot reviewed Nov 3, 2023

View reviewed changes

Merge branch 'feature/anteKeysChecks' of github.com:Zondax/cosmos-sdk…

7729e00

… into feature/anteKeysChecks

coderabbitai bot reviewed Nov 3, 2023

View reviewed changes

x/auth/ante/sigverify_test.go Show resolved Hide resolved

x/auth/ante/sigverify_test.go Show resolved Hide resolved

x/auth/ante/sigverify_test.go Show resolved Hide resolved

alexanderbez approved these changes Nov 7, 2023

View reviewed changes

bizk added this pull request to the merge queue Nov 7, 2023

Merged via the queue into cosmos:main with commit 346044a Nov 7, 2023
56 of 58 checks passed

bizk deleted the feature/anteKeysChecks branch November 7, 2023 13:32

tac0turtle reviewed Nov 7, 2023

View reviewed changes

coderabbitai bot mentioned this pull request Nov 21, 2023

feat: secp256k1 public key constant time #18026

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extra checks on signatures/pubkeys + check the signature first in antehandle #18194

feat: extra checks on signatures/pubkeys + check the signature first in antehandle #18194

bizk commented Oct 20, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 23, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

bizk commented Nov 2, 2023

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

tac0turtle Nov 7, 2023

bizk Nov 8, 2023

tac0turtle Nov 7, 2023

bizk Nov 8, 2023

tac0turtle left a comment

bizk commented Nov 8, 2023

feat: extra checks on signatures/pubkeys + check the signature first in antehandle #18194

feat: extra checks on signatures/pubkeys + check the signature first in antehandle #18194

Conversation

bizk commented Oct 20, 2023 • edited by coderabbitai bot Loading

Description

Author Checklist

Reviewers Checklist

Summary by CodeRabbit

coderabbitai bot commented Oct 23, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

bizk commented Nov 2, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

tac0turtle Nov 7, 2023

Choose a reason for hiding this comment

bizk Nov 8, 2023

Choose a reason for hiding this comment

tac0turtle Nov 7, 2023

Choose a reason for hiding this comment

bizk Nov 8, 2023

Choose a reason for hiding this comment

tac0turtle left a comment

Choose a reason for hiding this comment

bizk commented Nov 8, 2023

bizk commented Oct 20, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 23, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)